refactor(timeline): freeze-frame uses shared persistGeneratedMediaAsset path#257
Conversation
The hand-rolled persist flow in insertFreezeFrame duplicated logic that already lives in mediaLibraryService.importGeneratedImage (which wraps persistGeneratedMediaAsset). The hand-rolled version had been patched twice this audit — once for the createMedia-before-thumbnailId ordering bug, once for the prepend-before-execute orphan — and STILL didn't roll back the on-disk record if a later step failed. The shared helper has all of those right by construction: - opfsService.saveFile (file bytes to OPFS) - saveThumbnail (thumbnail blob + id) - mediaMetadata.thumbnailId stamped BEFORE createMedia - createMedia (persisted with thumbnailId) - writeMediaSource (background workspace mirror) - associateMediaWithProject - rollback of all of the above on any throw Switched insertFreezeFrame to wrap the canvas blob in a File and call mediaLibraryService.importGeneratedImage. The returned MediaMetadata is the same object we prepend to the store on success. Also added a rollback hop on the execute(...)===false branch: if the internal _splitItem returns null (rare race — source clip deleted between validation and execute), call mediaLibraryService.deleteMedia to clear the persisted frame so we don't leak an on-disk orphan. Net: -26 lines, deletes 4 imports (MediaMetadata, ThumbnailData, opfsService, writeMediaSource and the direct @/infrastructure/storage imports), inherits proper rollback for free. Verified: tsc clean, lint 0/0, 134 test files / 804 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors insertFreezeFrame to persist extracted frames via mediaLibraryService.importGeneratedImage(), switches imports to use timeline deps, acquires a blob URL for the new media, and adds rollback + blob URL release on timeline mutation failure. ChangesFreeze-frame media persistence refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryReplaces the hand-rolled OPFS/thumbnail/metadata persist flow in
Confidence Score: 3/5The happy path is clean and the refactor correctly delegates to the shared helper. The failure branch introduced in this PR is incomplete: it cleans up the on-disk asset but leaves the frame blob pinned in blobUrlManager, so every occurrence of the rare split-failure race leaks memory until the session ends. The core refactor is sound and removes previously buggy hand-rolled persistence. The new rollback branch in the !success path calls deleteMedia without a matching blobUrlManager.release, meaning the frame Blob stays alive in memory. Since this PR explicitly introduces that rollback branch, the omission is a present defect on the changed path. src/features/timeline/stores/actions/edit/freeze-frame-actions.ts — specifically the !success rollback block around line 222. Important Files Changed
Sequence DiagramsequenceDiagram
participant FF as insertFreezeFrame
participant MLS as mediaLibraryService
participant PGA as persistGeneratedMediaAsset
participant BUM as blobUrlManager
participant EX as execute(_splitItem)
FF->>MLS: importGeneratedImage(frameFile, projectId, opts)
MLS->>PGA: persistGeneratedMediaAsset(...)
PGA->>PGA: opfsService.saveFile
PGA->>PGA: saveThumbnail
PGA->>PGA: createMedia (with thumbnailId)
PGA->>PGA: writeMediaSource (fire-and-forget)
PGA->>PGA: associateMediaWithProject
PGA-->>MLS: MediaMetadata
MLS-->>FF: MediaMetadata
FF->>BUM: acquire(frameMediaId, frameBlob)
BUM-->>FF: frameBlobUrl
FF->>EX: execute("INSERT_FREEZE_FRAME", ...)
alt split succeeds
EX-->>FF: true
FF->>FF: prependMediaItem(mediaMetadata)
FF-->>FF: return true
else split fails (rare race)
EX-->>FF: false
Note over FF,BUM: blobUrlManager.release() missing here
FF->>MLS: deleteMedia(frameMediaId)
MLS-->>FF: void
FF-->>FF: return false
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/features/timeline/stores/actions/edit/freeze-frame-actions.ts:222-235
Missing `blobUrlManager` release in the `!success` rollback. `blobUrlManager.acquire(frameMediaId, frameBlob)` is called on line 144 before `execute()`. When the split returns `false`, `deleteMedia` properly cleans up the on-disk record, but the blob URL entry (and its underlying `Blob` reference) is never released from `blobUrlManager`. On every occurrence of this failure path the frame bytes stay pinned in memory indefinitely.
```suggestion
if (!success) {
// Roll back the persisted media so a failed split (rare — only if the
// source clip was deleted between validation and execute) doesn't leave
// an orphan on disk.
blobUrlManager.release(frameMediaId)
try {
await mediaLibraryService.deleteMedia(frameMediaId)
} catch (cleanupError) {
getLogger().warn(
'[insertFreezeFrame] Failed to roll back persisted frame after split failure',
cleanupError,
)
}
return false
}
```
### Issue 2 of 2
src/features/timeline/stores/actions/edit/freeze-frame-actions.ts:227
`deleteMedia` is documented as the "delete everywhere / no-project-context" variant. Its JSDoc explicitly says: *"Prefer `deleteMediaFromProject(projectId, mediaId)` when a project context exists: it preserves the media for other projects via reference counting."* Since `currentProjectId` is already in scope here, `deleteMediaFromProject` is the correct call — it cleans up the same single association without violating the API contract.
```suggestion
await mediaLibraryService.deleteMediaFromProject(currentProjectId, frameMediaId)
```
Reviews (1): Last reviewed commit: "refactor(timeline): freeze-frame uses pe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!success) { | ||
| // Roll back the persisted media so a failed split (rare — only if the | ||
| // source clip was deleted between validation and execute) doesn't leave | ||
| // an orphan on disk. | ||
| try { | ||
| await mediaLibraryService.deleteMedia(frameMediaId) | ||
| } catch (cleanupError) { | ||
| getLogger().warn( | ||
| '[insertFreezeFrame] Failed to roll back persisted frame after split failure', | ||
| cleanupError, | ||
| ) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
blobUrlManager release in the !success rollback. blobUrlManager.acquire(frameMediaId, frameBlob) is called on line 144 before execute(). When the split returns false, deleteMedia properly cleans up the on-disk record, but the blob URL entry (and its underlying Blob reference) is never released from blobUrlManager. On every occurrence of this failure path the frame bytes stay pinned in memory indefinitely.
| if (!success) { | |
| // Roll back the persisted media so a failed split (rare — only if the | |
| // source clip was deleted between validation and execute) doesn't leave | |
| // an orphan on disk. | |
| try { | |
| await mediaLibraryService.deleteMedia(frameMediaId) | |
| } catch (cleanupError) { | |
| getLogger().warn( | |
| '[insertFreezeFrame] Failed to roll back persisted frame after split failure', | |
| cleanupError, | |
| ) | |
| } | |
| return false | |
| } | |
| if (!success) { | |
| // Roll back the persisted media so a failed split (rare — only if the | |
| // source clip was deleted between validation and execute) doesn't leave | |
| // an orphan on disk. | |
| blobUrlManager.release(frameMediaId) | |
| try { | |
| await mediaLibraryService.deleteMedia(frameMediaId) | |
| } catch (cleanupError) { | |
| getLogger().warn( | |
| '[insertFreezeFrame] Failed to roll back persisted frame after split failure', | |
| cleanupError, | |
| ) | |
| } | |
| return false | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/timeline/stores/actions/edit/freeze-frame-actions.ts
Line: 222-235
Comment:
Missing `blobUrlManager` release in the `!success` rollback. `blobUrlManager.acquire(frameMediaId, frameBlob)` is called on line 144 before `execute()`. When the split returns `false`, `deleteMedia` properly cleans up the on-disk record, but the blob URL entry (and its underlying `Blob` reference) is never released from `blobUrlManager`. On every occurrence of this failure path the frame bytes stay pinned in memory indefinitely.
```suggestion
if (!success) {
// Roll back the persisted media so a failed split (rare — only if the
// source clip was deleted between validation and execute) doesn't leave
// an orphan on disk.
blobUrlManager.release(frameMediaId)
try {
await mediaLibraryService.deleteMedia(frameMediaId)
} catch (cleanupError) {
getLogger().warn(
'[insertFreezeFrame] Failed to roll back persisted frame after split failure',
cleanupError,
)
}
return false
}
```
How can I resolve this? If you propose a fix, please make it concise.
| // source clip was deleted between validation and execute) doesn't leave | ||
| // an orphan on disk. | ||
| try { | ||
| await mediaLibraryService.deleteMedia(frameMediaId) |
There was a problem hiding this comment.
deleteMedia is documented as the "delete everywhere / no-project-context" variant. Its JSDoc explicitly says: "Prefer deleteMediaFromProject(projectId, mediaId) when a project context exists: it preserves the media for other projects via reference counting." Since currentProjectId is already in scope here, deleteMediaFromProject is the correct call — it cleans up the same single association without violating the API contract.
| await mediaLibraryService.deleteMedia(frameMediaId) | |
| await mediaLibraryService.deleteMediaFromProject(currentProjectId, frameMediaId) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/timeline/stores/actions/edit/freeze-frame-actions.ts
Line: 227
Comment:
`deleteMedia` is documented as the "delete everywhere / no-project-context" variant. Its JSDoc explicitly says: *"Prefer `deleteMediaFromProject(projectId, mediaId)` when a project context exists: it preserves the media for other projects via reference counting."* Since `currentProjectId` is already in scope here, `deleteMediaFromProject` is the correct call — it cleans up the same single association without violating the API contract.
```suggestion
await mediaLibraryService.deleteMediaFromProject(currentProjectId, frameMediaId)
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/timeline/stores/actions/edit/freeze-frame-actions.ts (1)
239-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOuter catch does not roll back media persisted earlier in the try block.
If anything between line 143 (
importGeneratedImagereturned successfully) and theexecuteflow throws — e.g.blobUrlManager.acquirerejects,executeitself throws rather than returning false, or any of the Zustand mutations inside the callback throws — we land here without invokingdeleteMediaonframeMediaId, so the persisted file + thumbnail + project association stay on disk while the timeline is unchanged.frameMediaIdis also block-scoped inside thetry, so even adding cleanup here would need it hoisted.🛡️ Proposed fix — hoist the id and mirror the rollback in the catch
- try { + let frameMediaId: string | undefined + try { // ...existing extraction/persist flow... - const frameMediaId = mediaMetadata.id + frameMediaId = mediaMetadata.id const frameBlobUrl = blobUrlManager.acquire(frameMediaId, frameBlob) // ... } catch (error) { getLogger().error('[insertFreezeFrame] Failed:', error) + if (frameMediaId) { + try { + await mediaLibraryService.deleteMedia(frameMediaId) + blobUrlManager.release?.(frameMediaId) + } catch (cleanupError) { + getLogger().warn( + '[insertFreezeFrame] Failed to roll back persisted frame after error', + cleanupError, + ) + } + } return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/timeline/stores/actions/edit/freeze-frame-actions.ts` around lines 239 - 242, Hoist the variable that holds the imported media id (currently created by importGeneratedImage) out of the try block so it’s available in the outer catch, and in the catch for insertFreezeFrame mirror the same rollback used on failure paths: if frameMediaId is set call deleteMedia(frameMediaId) (and any thumbnail/project cleanup you normally do) before returning false; ensure you also only attempt rollback when frameMediaId is truthy and handle errors from deleteMedia safely. Make these changes around the importGeneratedImage call, blobUrlManager.acquire usage, and the execute(...) flow so any thrown/rejected errors between import and execute trigger the same cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/timeline/stores/actions/edit/freeze-frame-actions.ts`:
- Around line 222-235: The rollback path in insertFreezeFrame currently deletes
persisted media via mediaLibraryService.deleteMedia(frameMediaId) but never
releases the object URL acquired earlier with
blobUrlManager.acquire(frameMediaId, frameBlob), leaking the blob URL; update
the rollback to call blobUrlManager.release(frameMediaId) (ideally in a finally
block so it runs even if deleteMedia throws) referencing insertFreezeFrame,
frameMediaId, and blobUrlManager.release to ensure the acquired URL is always
released during failure cleanup.
---
Outside diff comments:
In `@src/features/timeline/stores/actions/edit/freeze-frame-actions.ts`:
- Around line 239-242: Hoist the variable that holds the imported media id
(currently created by importGeneratedImage) out of the try block so it’s
available in the outer catch, and in the catch for insertFreezeFrame mirror the
same rollback used on failure paths: if frameMediaId is set call
deleteMedia(frameMediaId) (and any thumbnail/project cleanup you normally do)
before returning false; ensure you also only attempt rollback when frameMediaId
is truthy and handle errors from deleteMedia safely. Make these changes around
the importGeneratedImage call, blobUrlManager.acquire usage, and the
execute(...) flow so any thrown/rejected errors between import and execute
trigger the same cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04638b20-63bf-48db-ad74-a6a9ddaae697
📒 Files selected for processing (1)
src/features/timeline/stores/actions/edit/freeze-frame-actions.ts
Two issues raised by reviewers on the freeze-frame persist refactor: 1. P1 — Blob URL leak on rollback. blobUrlManager.acquire(frameMediaId, frameBlob) ran before execute(); when split returned false we called deleteMedia but never released the blob URL entry. Each failure would accumulate a leaked Blob in memory. Added matching blobUrlManager.release(frameMediaId) in the !success branch. 2. P2 — Use deleteMediaFromProject not deleteMedia. The frame was just associated with currentProjectId and is only referenced by this project, so the reference-counted variant is the right call. The docstring for deleteMedia explicitly says to prefer deleteMediaFromProject when project context exists. Verified: tsc clean, lint 0/0, 113 test files / 648 timeline tests pass.
1 participant
Summary
The hand-rolled persist flow in `insertFreezeFrame` duplicated logic that already lives in `mediaLibraryService.importGeneratedImage` (which wraps `persistGeneratedMediaAsset` at `media-asset-helpers.ts:109`). The hand-rolled version had been patched twice during this audit:
…and still didn't roll back the on-disk record if a later step threw. The shared helper has all of those right by construction:
Switched `insertFreezeFrame` to wrap the canvas blob in a `File` and call `mediaLibraryService.importGeneratedImage`. The returned `MediaMetadata` is the same object we prepend to the store on success.
Also added a rollback hop on the `execute(...) === false` branch: if the inner `_splitItem` returns null (rare race — source clip deleted between validation and execute), call `mediaLibraryService.deleteMedia` to clear the persisted frame so we don't leak an on-disk orphan.
Diff shape
Test plan
Summary by CodeRabbit